Skip to content

Conversation

@rxin
Copy link
Contributor

@rxin rxin commented Jul 6, 2016

What changes were proposed in this pull request?

This is a small follow-up for SPARK-16371:

  1. Hide removeMetadata from public API.
  2. Add JIRA ticket number to test case name.

How was this patch tested?

Updated a test comment.

@SparkQA
Copy link

SparkQA commented Jul 6, 2016

Test build #61862 has finished for PR 14074 at commit 29af80e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor Author

rxin commented Jul 6, 2016

Merging in master/2.0 - this is basically my code review feedback on an existing pr.

@asfgit asfgit closed this in 8e3e4ed Jul 6, 2016
asfgit pushed a commit that referenced this pull request Jul 6, 2016
## What changes were proposed in this pull request?
This is a small follow-up for SPARK-16371:

1. Hide removeMetadata from public API.
2. Add JIRA ticket number to test case name.

## How was this patch tested?
Updated a test comment.

Author: Reynold Xin <rxin@databricks.com>

Closes #14074 from rxin/parquet-filter.

(cherry picked from commit 8e3e4ed)
Signed-off-by: Reynold Xin <rxin@databricks.com>
}

test("Do not push down filters incorrectly when inner name and outer name are the same") {
test("SPARK-16371 Do not push down filters when inner name and outer name are the same") {
Copy link
Member

@HyukjinKwon HyukjinKwon Jul 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rxin, I am sorry for asking such a question but I couldn't find the documentation for this.

I am sometimes confused if I should add the JIRA number as prefix of a test or not. I am trying to comply this rule which I think is, writing single test requires to add the JIRA number in the test name and writing multiple tests does not (per-PR, per-JIRA).

Is this just encouraged to write this always? If so, I will try to comply this from now on and will help you leave such comments when I happened to look at some other PRs.

(I don't intend to change this across codebase but the main reason why I am asking is, that I don't want to make a such mistake in the future..)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we are creating a test case and a patch to fix specifically a problem, it'd be good to have that in the name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants